r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Some changes occurred in src/tools/compiletest

cc @jieyouxu

LGTM from the compiletest side once the pretty printer files changes are removed, but I don't have a Windows system to check if the above comment about the linker is true
r? compiler

I also don't have windows
r? bjorn3
I think I remember you knowing things about windows?

I haven't really touched Windows in years and never did any native development on it. I know some linkage related things because I was trying to get weak linkage working on Windows for EII. As such I don't think I'm a suitable reviewer.

Hmm... @michaelwoerister reviewed this code originally. Any thoughts?

rust/compiler/rustc_codegen_ssa/src/back/linker.rs

Lines 1087 to 1105 in a3903b1

	// This will cause the Microsoft linker to embed .natvis info into the PDB file
	let natvis_dir_path = self.sess.opts.sysroot.path().join("lib\\rustlib\\etc");
	if let Ok(natvis_dir) = fs::read_dir(&natvis_dir_path) {
	for entry in natvis_dir {
	match entry {
	Ok(entry) => {
	let path = entry.path();
	if path.extension() == Some("natvis".as_ref()) {
	let mut arg = OsString::from("/NATVIS:");
	arg.push(path);
	self.link_arg(arg);
	}
	}
	Err(error) => {
	self.sess.dcx().emit_warn(errors::NoNatvisDirectory { error });
	}
	}
	}
	}

can be removed after these changes, right?

Looking into it, this appears to be the case; the loop below this section that collects the natvis files from all compiled crates should be sufficient. Removed it.

This looks right to me. The only possible downside I can think of is that this now loads all the natvis files into memory and then writes them out to temporary files (with more obscure file names). But a) the size is not large and b) that's just like how user natvis files are treated anyway. So I don't think it's much of a downside. If it is advantageous to optimise this then such an optimisation should be applied outside of the standard library too.

I would be truly shocked if 14KiB of XML was what broke the bank for compiling on Windows.

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

I guess this will need updating:

Or rust-windbg.cmd could be removed completely given that it's presumably not useful anymore and rustup doesn't even install a forwarder for it.

Wow, that script hasn't been touched in 8 years. I'm going to leave it in this PR but would say it's fair to remove if you want to try submitting a PR that removes it and seeing who objects.

@bors r=bjorn3,ChrisDenton rollup

📌 Commit d6bf47d has been approved by bjorn3,ChrisDenton

It is now in the queue for this repository.

@bors r-

Right, brain fart. That script should be modified too.

This pull request was unapproved.

View changes since this unapproval

Gonna defer to the incredibly democratic and sound process of asking Windows users on Zulip if they care: #general > rust-windbg script?

Kind of agree that the best course of action would be to just remove the script and update the dev guide, which appear to be the only relevant places rust-windbg is even mentioned. Dev guide PR is rust-lang/rustc-dev-guide#2894

I admit I forgot rust-windbg.cmd exists but I'd be surprised if nobody was using it, even if it's a pain to find.

Since this was suggested:

@rustbot ping windows

But personally, I think to get this out of the door for now, I like the idea of just leaving a message in the script and waiting for some predetermined amount of time to see if anyone notices. I assume that the answer will just be to delete the script, but figure people are still using it.

Hey Windows Group! This issue has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
issues. Maybe take a look?
Thanks! <3

cc @albertlarsan68 @arlosi @ChrisDenton @danielframpton @dpaoliello @gdr-at-ms @kennykerr @luqmana @nico-abram @retep998 @sivadeilra @wesleywiser

Just a heads up, CI probably won't catch any issues with this. AFAIK, the only semi-reliable test runner for debug info is aarch64-apple. As of a day or 2 ago, running tests/debuginfo on windows MSVC results in 56 failed tests on my computer. Tbh, I don't really trust the tests that do pass either. Many have been disabled for not working rather than being properly fixed. The tests that are left don't exercise everything the visualizers output, nor do they test the inputs very well. I'm working on fixing that as part of GSoC, but microsoft's debugger is lowest on the priority list unfortunately.

All that said, imo this 100% needs to be tested by hand by somebody before it gets merged.

I've given my general sentiment on this change on zulip (tl;dr, i would prefer almost any alternative that doesn't use #![debugger_visualizer] within Rust itself). If the consensus is that this is the best solution though, I have a windows machine I can test this on.